Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arbitrary self types v2: main compiler changes #132961

Merged
merged 6 commits into from
Dec 13, 2024

Conversation

adetaylor
Copy link
Contributor

This is the main PR in a series of PRs related to Arbitrary Self Types v2, tracked in #44874. Specifically this is step 7 of the plan described here, for RFC 3519.

Overall this PR:

  • Switches from the Deref trait to the new Receiver trait when the unstable arbitrary_self_types feature is enabled (the simple bit)
  • Introduces new algorithms to spot "shadowing"; that is, the case where a newly-added method in an outer smart pointer might end up overriding a pre-existing method in the pointee (the complex bit). Most of this bit was explored in this earlier perf-testing PR.
  • Lots of tests

This should not break compatibility for:

  • Stable users, where it should have no effect
  • Users of the existing arbitrary_self_types feature (because we implement Receiver for T: Deref) unless those folks have added methods which may shadow methods in inner types, which we no longer want to allow

Subsequent PRs will add better diagnostics.

It's probably easiest to review this commit-by-commit.

r? @wesleywiser

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 12, 2024
@adetaylor
Copy link
Contributor Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2024
@rust-log-analyzer

This comment has been minimized.

@adetaylor
Copy link
Contributor Author

The failure here is interesting - it seems that our deshadowing algorithm does impact some existing code. IndexVec has its own into_iter() and so might its contents have (it implements Deref). It was not intentional that this deshadowing impacts existing code but I think I can see why the pattern here wasn't what I was expecting - I'll need to do a bit of thinking about how to avoid this case.

@rust-log-analyzer

This comment has been minimized.

@adetaylor adetaylor force-pushed the arbitrary-self-types-the-big-bit branch from 0e03674 to adea5bc Compare November 14, 2024 14:50
compiler/rustc_error_codes/src/error_codes/E0307.md Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/wfcheck.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/probe.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/traits/query.rs Outdated Show resolved Hide resolved
// ```
// In these circumstances, the logic is wrong, and we wouldn't spot
// the shadowing, because the autoderef-based maths wouldn't line up.
// This is a niche case and we can live without generating an error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe this should be an unresolved question on the tracking issue. T-lang can decide if they're fine with this niche case before stabilization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll wait until we're largely through the review process here, and after we've done a crater run, then accumulate a list of questions which have come up, including this one.

Other assumptions I've made which might merit wider discussion:

  • We only care to apply the shadowing algorithm for inherent methods, not trait methods. (This applies both for the shadower and the shadowed method). This is the primary case that we cared about in RFC discussions, so I've gone with that for simplicity, but there are also arguments that we might want to error if (for example) trait methods are shadowed by a new inherent method, or even vice-versa.
  • We don't want to try to spot shadowing problems involving raw pointers or the newfangled "reborrowed pin" (which didn't exist when we wrote the RFC)

@adetaylor
Copy link
Contributor Author

Thanks for the early review @wesleywiser - I have a question for you as well - obviously, for now, the new functionality remains behind the arbitrary_self_types feature flag. Eventually we want to stabilize it, and it would be good to know now about any unforeseen problems there, before this PR is finalized and merged. Is there best practice for raising a PR and/or a crater run to test the world as if the feature were stabilized?

@adetaylor adetaylor force-pushed the arbitrary-self-types-the-big-bit branch 2 times, most recently from c4bf39f to c0e8581 Compare November 20, 2024 18:23
@adetaylor
Copy link
Contributor Author

(NB this is still in draft - no need to review further till I put it up for proper review - there's still work to do)

@adetaylor adetaylor force-pushed the arbitrary-self-types-the-big-bit branch from c0e8581 to 520f90b Compare November 20, 2024 18:30
@wesleywiser
Copy link
Member

If you would like the crater run done, what I would suggest is opening a second PR that has the changes you want to test un-feature-gated (ie, as if they had been stabilized) and then we can do a crater run on that PR and close it once the results come back.

@adetaylor
Copy link
Contributor Author

adetaylor commented Nov 21, 2024

If you would like the crater run done, what I would suggest is opening a second PR that has the changes you want to test un-feature-gated (ie, as if they had been stabilized) and then we can do a crater run on that PR and close it once the results come back.

Great - I'll do that. (Update: done in #133570)

@adetaylor adetaylor force-pushed the arbitrary-self-types-the-big-bit branch 3 times, most recently from 6d61ab8 to 061c77c Compare November 22, 2024 17:19
@adetaylor
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2024
@adetaylor adetaylor marked this pull request as ready for review November 22, 2024 22:04
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 22, 2024
@adetaylor adetaylor force-pushed the arbitrary-self-types-the-big-bit branch from 061c77c to 560a529 Compare November 28, 2024 13:13
@compiler-errors
Copy link
Member

Anyways, r=me is @wesleywiser is okay with these changes.

@wesleywiser
Copy link
Member

There's another crater run queued in #133570 to investigate the Unknown Results but for now

@bors r=compiler-errors,wesleywiser

@bors
Copy link
Contributor

bors commented Dec 10, 2024

📌 Commit 718b4ca has been approved by compiler-errors,wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2024
@bors
Copy link
Contributor

bors commented Dec 10, 2024

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout arbitrary-self-types-the-big-bit (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self arbitrary-self-types-the-big-bit --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: tests/ui/self/arbitrary-self-opaque.stderr (HEAD vs. heads/homu-tmp)
Auto-merging tests/ui/self/arbitrary-self-opaque.stderr
CONFLICT (content): Merge conflict in tests/ui/self/arbitrary-self-opaque.stderr
Auto-merging compiler/rustc_hir_analysis/src/check/wfcheck.rs
Auto-merging compiler/rustc_hir_analysis/messages.ftl
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 10, 2024
In this new version of Arbitrary Self Types, we no longer use the Deref trait
exclusively when working out which self types are valid. Instead, we follow a
chain of Receiver traits. This enables methods to be called on smart pointer
types which fundamentally cannot support Deref (for instance because they are
wrappers for pointers that don't follow Rust's aliasing rules).

This includes:
* Changes to tests appropriately
* New tests for:
  * The basics of the feature
  * Ensuring lifetime elision works properly
  * Generic Receivers
  * A copy of the method subst test enhanced with Receiver

This is really the heart of the 'arbitrary self types v2' feature, and
is the most critical commit in the current PR.

Subsequent commits are focused on:
* Detecting "shadowing" problems, where a smart pointer type can hide
  methods in the pointee.
* Diagnostics and cleanup.

Naming: in this commit, the "Autoderef" type is modified so that it no
longer solely focuses on the "Deref" trait, but can now consider the
"Receiver" trait instead. Should it be renamed, to something like
"TraitFollower"? This was considered, but rejected, because
* even in the Receiver case, it still considers built-in derefs
* the name Autoderef is short and snappy.
This commit makes no (intentional) functional change.

Previously, the picking process maintained two lists of extra
information useful for diagnostics:

* any unstable candidates which might have been picked
* any unsatisfied predicates

Previously, these were dealt with quite differently - the former list
was passed around as a function parameter; the latter lived in a RefCell
in the ProbeCtxt.

With this change we increase consistency by keeping them together in a
new PickDiagHints structure, passed as a parameter, with no need for
interior mutability.

The lifecycle of each of these lists remains fairly complex, so it's
explained with new comments in pick_core.

A further cleanup here would be to package the widely-used tuple
  (ty::Predicate<'tcx>,
   Option<ty::Predicate<'tcx>>,
   Option<ObligationCause<'tcx>>)
into a named struct for UnsatisfiedPredicate. This seems worth doing but
it turns out that this tuple is used in dozens of places, so if we're
going to do this we should do it as a separate PR to avoid constant
rebase trouble.
This is the first part of a series of commits which impact the
"deshadowing detection" in the arbitrary self types v2 RFC.

This commit should not have any functional changes, but may impact
performance. Subsequent commits add back the performance, and add error
checking to this new code such that it has a functional effect.

Rust prioritizes method candidates in this order:
1. By value;
2. By reference;
3. By mutable reference;
4. By const ptr.
5. By reborrowed pin.

Previously, if a suitable candidate was found in one of these earlier
categories, Rust wouldn't even move onto probing the other categories.

As part of the arbitrary self types work, we're going to need to change
that - even if we choose a method from one of the earlier categories, we
will sometimes need to probe later categories to search for methods that
we may be shadowing.

This commit adds those extra searches for shadowing, but it does not yet
produce an error when such shadowing problems are found. That will come
in a subsequent commit, by filling out the 'check_for_shadowing'
method.

This commit contains a naive approach to detecting these shadowing
problems, which shows what we've functionally looking to do. However,
it's too slow. The performance of this approach was explored in this
PR:
rust-lang#127812 (comment)
Subsequent commits will improve the speed of the search.
A previous commit added a search for certain types of "shadowing"
situation where one method (in an outer smart pointer type, typically)
might hide or shadow the method in the pointee.

Performance investigation showed that the naïve approach is too slow -
this commit speeds it up, while being functionally the same.

This still does not actually cause the deshadowing check to emit any
errors; that comes in a subsequent commit which is where all the tests
live.
This builds on the previous commits by actually adding checks for cases
where a new method shadows an older method.
There's some discussion on the RFC about whether generic receivers should be
allowed, but in the end the conclusion was that they should be blocked
(at least for some definition of 'generic'). This blocking landed in
an earlier PR; this commit adds additional tests to ensure the
interaction with the rest of the Arbitrary Self Types v2 feature is as
expected. This test may be a little duplicative but it seems better
to land it than not.
@adetaylor adetaylor force-pushed the arbitrary-self-types-the-big-bit branch from 718b4ca to 337af8a Compare December 11, 2024 11:59
@compiler-errors
Copy link
Member

@bors r=compiler-errors,wesleywiser

@bors
Copy link
Contributor

bors commented Dec 11, 2024

📌 Commit 337af8a has been approved by compiler-errors,wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2024
@bors
Copy link
Contributor

bors commented Dec 12, 2024

⌛ Testing commit 337af8a with merge 915e7eb...

@bors
Copy link
Contributor

bors commented Dec 13, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors,wesleywiser
Pushing 915e7eb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2024
@bors bors merged commit 915e7eb into rust-lang:master Dec 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 13, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (915e7eb): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.4%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 4.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (primary -3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.2% [-3.2%, -3.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.2% [-3.2%, -3.2%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.02s -> 769.736s (-0.04%)
Artifact size: 331.01 MiB -> 331.09 MiB (0.02%)

@rustbot rustbot removed the perf-regression Performance regression. label Dec 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 13, 2024
…mpiler-errors

Arbitrary self types v2: adjust diagnostic.

The recently landed PR rust-lang#132961 to adjust arbitrary self types was a bit overenthusiastic, advising folks to use the new Receiver trait even before it's been stabilized. Revert to the older wording of the lint in such cases.

Tracking issue rust-lang#44874

r? `@wesleywiser`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2024
…mpiler-errors

Arbitrary self types v2: adjust diagnostic.

The recently landed PR rust-lang#132961 to adjust arbitrary self types was a bit overenthusiastic, advising folks to use the new Receiver trait even before it's been stabilized. Revert to the older wording of the lint in such cases.

Tracking issue rust-lang#44874

r? ``@wesleywiser``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Rollup merge of rust-lang#134262 - adetaylor:revert-diagnostics, r=compiler-errors

Arbitrary self types v2: adjust diagnostic.

The recently landed PR rust-lang#132961 to adjust arbitrary self types was a bit overenthusiastic, advising folks to use the new Receiver trait even before it's been stabilized. Revert to the older wording of the lint in such cases.

Tracking issue rust-lang#44874

r? ``@wesleywiser``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants